Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[REMOVAL] Remove the jaeger exporter #2031

Merged
merged 21 commits into from
Jul 2, 2023
Merged

[REMOVAL] Remove the jaeger exporter #2031

merged 21 commits into from
Jul 2, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Mar 8, 2023 •
edited
Loading

Fixes #1938

Changes

  • Remove the jaeger exporter
  • Remove usage of thrift
  • Remove usage of boost

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Mar 8, 2023 •
edited
Loading

Codecov Report

Merging #2031 (db7396f) into main (049ab63) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2031      +/-   ##
==========================================
- Coverage   87.53%   87.51%   -0.02%     
==========================================
  Files         169      169              
  Lines        4889     4889              
==========================================
- Hits         4279     4278       -1     
- Misses        610      611       +1

see 1 file with indirect coverage changes

@marcalff marcalff marked this pull request as ready for review March 8, 2023 22:01
@marcalff marcalff requested a review from a team March 8, 2023 22:01
esigo
esigo approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for the PR :)

@esigo esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Mar 9, 2023
ThomsonTan
ThomsonTan reviewed Mar 9, 2023
View reviewed changes
CMakeLists.txt
@@ -175,8 +175,6 @@ option(WITH_ELASTICSEARCH

option(WITH_ZPAGES "Whether to include the Zpages Server in the SDK" OFF)

option(WITH_JAEGER "DEPRECATED - Whether to include the Jaeger exporter" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the option but only allow the default value? I mean if we delete the option, the existing cmake invocation like cmake -DWITH_JAEGER=OFF will have warning triggered. The same applies to cmake -DWITH_JAEGER=ON. Better to show an explicit error message the user tries to enable Jaeger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the option but only allow the default value? I mean if we delete the option, the existing cmake invocation like cmake -DWITH_JAEGER=OFF will have warning triggered. The same applies to cmake -DWITH_JAEGER=ON. Better to show an explicit error message the user tries to enable Jaeger?

Thanks for the early comments.

The initial deprecation was announced with release 1.8.2 published on 2023-01-31.
It was advertised at great length, including with pinned issues, so users should be well aware of this by now.

With this removal, the following commands:

cmake -DWITH_JAEGER=ON
cmake -DWITH_JAEGER=OFF

will both produce a warning, with CMake indicating that an unknown option is used:

CMake Warning:
  Manually-specified variables were not used by the project:

    WITH_JAEGER

Using -DWITH_JAEGER=OFF produces a warning only, and will not cause the build to fail.

The fact that the WITH_JAEGER option is no longer available is now documented in the CHANGELOG, in the important changes section, for the next release.

I don't think the CMakeList.txt should still contain logic about obsolete options, as it makes removing options impossible, this is mitigated by the important changes in the CHANGELOG instead.

@lalitb
Copy link
Member

lalitb commented Mar 10, 2023

Thanks for the PR. Changes look good. Was trying to see if the specs says anything about removing support of Jaeger . There are couple of reference -

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#jaeger-exporter

Jaeger exporter support will be removed from OpenTelemetry in July 2023.

And changelog - https://github.com/open-telemetry/opentelemetry-specification/blob/b674b49a9be32bf38a58865e9a04803c0f9349fb/CHANGELOG.md?plain=1#LL213C3-L213C70

- Deprecate jaeger exporter, scheduled for spec removal in July 2023.

Will it make sense to merge this PR when the Jaeger exporter reference is removed from specs ?

@lalitb lalitb mentioned this pull request Mar 10, 2023
Closed
@marcalff
Copy link
Member Author 

- Deprecate jaeger exporter, scheduled for spec removal in July 2023.

Will it make sense to merge this PR when the Jaeger exporter reference is removed from specs ?

Looks like I missed the date from the spec when writing the C++ deprecation PR, which is why the planned removal date for C++ was set to April, 2023 instead.

Ok to wait for July 2023 then.

@marcalff marcalff changed the title [REMOVAL] Remove the jeager exporter [ON HOLD UNTIL JULY 2023] [REMOVAL] Remove the jeager exporter Mar 10, 2023
@marcalff marcalff marked this pull request as draft March 10, 2023 09:22
@lalitb lalitb removed the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Mar 14, 2023
@marcalff marcalff mentioned this pull request Mar 27, 2023
Closed
@esigo esigo mentioned this pull request May 17, 2023
Closed
3 tasks
@marcalff marcalff marked this pull request as ready for review June 14, 2023 19:13
@marcalff marcalff changed the title [ON HOLD UNTIL JULY 2023] [REMOVAL] Remove the jeager exporter [REMOVAL] Remove the jaeger exporter Jun 14, 2023
@marcalff
Copy link
Member Author

Now ready for review.

@marcalff
Copy link
Member Author

TODO, waiting on spec changes.

The Jaeger Propagator will not be removed from the spec:

so it needs to be preserved in opentelemetry-cpp as well.

@marcalff marcalff added the pr:do-not-merge This PR is not ready to be merged. label Jun 27, 2023
@marcalff
Copy link
Member Author

do not merge yet, minor rework on Jaeger Propagator needed.

Will clear the do not merge flag once resolved.

@marcalff
Copy link
Member Author

Jaeger Propagator is no longer removed, per spec.

Updated DEPRECATED.md to indicate the propagator stays deprecated in opentelemetry-cpp (not removed)

@marcalff marcalff removed the pr:do-not-merge This PR is not ready to be merged. label Jun 27, 2023
ThomsonTan
ThomsonTan approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for driving this change, @marcalff

lalitb
lalitb approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nicely done, as usual :)

@marcalff marcalff merged commit 42709f4 into open-telemetry:main Jul 2, 2023
@chenrui333 chenrui333 mentioned this pull request Jul 14, 2023
Merged
@marcalff marcalff deleted the remove_jaeger_exporter_1938 branch September 5, 2023 15:08
@ThomsonTan ThomsonTan mentioned this pull request Jan 23, 2024
Merged
@CJCombrink CJCombrink mentioned this pull request Jun 14, 2024
Merged
3 tasks
littlewu2508 added a commit to littlewu2508/gentoo that referenced this pull request Aug 14, 2024
@littlewu2508
dev-cpp/opentelemetry-cpp: add 1.16.1
e8c835f 
IUSE=jager and dev-libs/thrift dependency is removed by
open-telemetry/opentelemetry-cpp#2031

Add remote-id to metadata

Bug: https://bugs.gentoo.org/900707
Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
littlewu2508 added a commit to littlewu2508/gentoo that referenced this pull request Aug 14, 2024
@littlewu2508
sys-cluster/ceph: limit opentelemetry-cpp version
d7a6326 
opentelemetry-cpp removes jaeger from v1.10.0 in
open-telemetry/opentelemetry-cpp#2031

Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
littlewu2508 added a commit to littlewu2508/gentoo that referenced this pull request Dec 1, 2024
@littlewu2508
dev-cpp/opentelemetry-cpp: add 1.16.1
87c0292 
IUSE=jager and dev-libs/thrift dependency is removed by
open-telemetry/opentelemetry-cpp#2031

Add remote-id to metadata

Bug: https://bugs.gentoo.org/900707
Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
littlewu2508 added a commit to littlewu2508/gentoo that referenced this pull request Dec 1, 2024
@littlewu2508
sys-cluster/ceph: limit opentelemetry-cpp version
eb2b2a7 
sys-cluster/ceph[jaeger] depends on opentelemetry-cpp[jaeger]
while opentelemetry-cpp removes jaeger from v1.10.0 in
open-telemetry/opentelemetry-cpp#2031

Remove 19.2.0 ebuild which should be replaced by its revision

Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Dec 1, 2024
@littlewu2508 @thesamesam
dev-cpp/opentelemetry-cpp: add 1.16.1
6adc7de 
IUSE=jager and dev-libs/thrift dependency is removed by
open-telemetry/opentelemetry-cpp#2031

Add remote-id to metadata

Bug: https://bugs.gentoo.org/900707
Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
Signed-off-by: Sam James <sam@gentoo.org>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Dec 1, 2024
@littlewu2508 @thesamesam
sys-cluster/ceph: limit opentelemetry-cpp version
38e68aa 
sys-cluster/ceph[jaeger] depends on opentelemetry-cpp[jaeger]
while opentelemetry-cpp removes jaeger from v1.10.0 in
open-telemetry/opentelemetry-cpp#2031

Remove 19.2.0 ebuild which should be replaced by its revision

Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
Closes: #38146
Signed-off-by: Sam James <sam@gentoo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esigo esigo esigo approved these changes

@lalitb lalitb lalitb approved these changes

@ThomsonTan ThomsonTan ThomsonTan approved these changes

Assignees

@ThomsonTan ThomsonTan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[REMOVAL] Remove the Jaeger Exporter
4 participants
@marcalff @lalitb @ThomsonTan @esigo